Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add screenshot to SentryFeedbackWidget #2369

Merged
merged 11 commits into from
Oct 29, 2024

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Oct 22, 2024

📜 Description

Add the option to add a screenshot SentryAttachment to the SentryFeedbackWidget

Bildschirmfoto 2024-10-22 um 17 20 47

💡 Motivation and Context

Closes to #1593

💚 How did you test it?

Unit tests, example app.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

Copy link
Contributor

github-actions bot commented Oct 22, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 8d54f66

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 52.38095% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.84%. Comparing base (0479083) to head (8d54f66).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
flutter/lib/src/sentry_flutter.dart 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2369      +/-   ##
==========================================
+ Coverage   84.78%   84.84%   +0.05%     
==========================================
  Files         253       79     -174     
  Lines        9070     2804    -6266     
==========================================
- Hits         7690     2379    -5311     
+ Misses       1380      425     -955     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denrase denrase changed the title Add screenshot wo feedback widget Add screenshot to SentryFeedbackWidget Oct 22, 2024
@denrase
Copy link
Collaborator Author

denrase commented Oct 22, 2024

@buenaflor I'm not sure if moving the code that takes the screenshot to the SentryScreenshotWidget is even neccessary, as the user facing API is SentryFlutter.captureFeedback. Pls add ur 2 cents.

@denrase denrase marked this pull request as ready for review October 22, 2024 15:39
Copy link
Contributor

github-actions bot commented Oct 22, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 467.14 ms 504.72 ms 37.58 ms
Size 6.49 MiB 7.57 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e2d89fc 323.84 ms 376.23 ms 52.39 ms
be173fa 375.94 ms 458.36 ms 82.42 ms
7b2e0ad 415.59 ms 457.26 ms 41.67 ms
322aa66 284.98 ms 341.76 ms 56.78 ms
eecbbca 324.37 ms 352.49 ms 28.12 ms
f3a18f2 456.29 ms 504.41 ms 48.12 ms
e0ba81f 390.38 ms 465.40 ms 75.02 ms
33527b4 403.58 ms 507.44 ms 103.86 ms
3334ac1 303.98 ms 366.65 ms 62.67 ms
5e7abc5 360.82 ms 401.18 ms 40.37 ms

App size

Revision Plain With Sentry Diff
e2d89fc 6.06 MiB 7.03 MiB 989.37 KiB
be173fa 6.35 MiB 7.33 MiB 1005.54 KiB
7b2e0ad 6.52 MiB 7.61 MiB 1.08 MiB
322aa66 5.94 MiB 6.92 MiB 1005.75 KiB
eecbbca 5.94 MiB 6.89 MiB 975.78 KiB
f3a18f2 6.49 MiB 7.57 MiB 1.08 MiB
e0ba81f 6.52 MiB 7.59 MiB 1.06 MiB
33527b4 6.35 MiB 7.42 MiB 1.07 MiB
3334ac1 6.06 MiB 7.03 MiB 993.54 KiB
5e7abc5 6.34 MiB 7.28 MiB 966.66 KiB

Previous results on branch: feat/add-screenshot-to-feedback-widget

Startup times

Revision Plain With Sentry Diff
10ecd8c 461.74 ms 480.69 ms 18.95 ms
be5c603 465.50 ms 505.94 ms 40.44 ms
6a14392 514.13 ms 562.09 ms 47.96 ms

App size

Revision Plain With Sentry Diff
10ecd8c 6.49 MiB 7.57 MiB 1.08 MiB
be5c603 6.49 MiB 7.57 MiB 1.08 MiB
6a14392 6.49 MiB 7.57 MiB 1.08 MiB

Copy link
Contributor

github-actions bot commented Oct 23, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1216.82 ms 1257.36 ms 40.55 ms
Size 8.38 MiB 9.75 MiB 1.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8cb6557 1265.14 ms 1266.08 ms 0.94 ms
bc29768 1247.25 ms 1268.63 ms 21.38 ms
3e5078c 1239.73 ms 1248.69 ms 8.96 ms
6957bfd 1283.80 ms 1289.00 ms 5.20 ms
0ceb89c 1252.02 ms 1271.78 ms 19.75 ms
d883d62 1221.39 ms 1230.18 ms 8.80 ms
1d47eb7 1212.57 ms 1222.00 ms 9.43 ms
21d4150 1252.86 ms 1280.55 ms 27.69 ms
6a5a65d 1237.22 ms 1250.29 ms 13.07 ms
b0811cc 1238.23 ms 1255.82 ms 17.59 ms

App size

Revision Plain With Sentry Diff
8cb6557 8.10 MiB 9.18 MiB 1.08 MiB
bc29768 8.32 MiB 9.38 MiB 1.05 MiB
3e5078c 8.28 MiB 9.34 MiB 1.06 MiB
6957bfd 8.15 MiB 9.15 MiB 1020.71 KiB
0ceb89c 8.15 MiB 9.12 MiB 989.78 KiB
d883d62 8.29 MiB 9.36 MiB 1.07 MiB
1d47eb7 8.29 MiB 9.39 MiB 1.09 MiB
21d4150 8.16 MiB 9.17 MiB 1.01 MiB
6a5a65d 8.34 MiB 9.65 MiB 1.31 MiB
b0811cc 8.32 MiB 9.38 MiB 1.06 MiB

Previous results on branch: feat/add-screenshot-to-feedback-widget

Startup times

Revision Plain With Sentry Diff
6a14392 1250.48 ms 1264.91 ms 14.43 ms
be5c603 1242.40 ms 1265.90 ms 23.50 ms
10ecd8c 1228.69 ms 1242.33 ms 13.63 ms

App size

Revision Plain With Sentry Diff
6a14392 8.38 MiB 9.75 MiB 1.37 MiB
be5c603 8.38 MiB 9.75 MiB 1.37 MiB
10ecd8c 8.38 MiB 9.75 MiB 1.37 MiB

@buenaflor
Copy link
Contributor

I'm not sure if moving the code that takes the screenshot to the SentryScreenshotWidget is even neccessary, as the user facing API is SentryFlutter.captureFeedback. Pls add ur 2 cents.

Can we keep it in the event processor but still capture the correct screen?

How does the JS user feedback behave in comparison

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

@denrase denrase merged commit 7954fb3 into main Oct 29, 2024
51 of 52 checks passed
@denrase denrase deleted the feat/add-screenshot-to-feedback-widget branch October 29, 2024 13:02
@bruno-garcia
Copy link
Member

Let me start with: Thanks! Love this!

Is this on the docs?
Does it support desktop and mobile? (rather: which platforms does it support?

We'd like to tell a holistic story about our feedback product and capabilities, and include all mobile support. @armcknight added support for iOS, and @romtsn had a PR for Android. More SDKs will come.

cc @jas-kas PM of User Feedback

martinhaintz added a commit that referenced this pull request Nov 11, 2024
* main:
  release: 8.10.1
  fix: android build error when compiling (#2397)
  release: 8.10.0
  chore: prepare changelog for `8.10.0` release (#2391)
  chore(deps): update Cocoa SDK to v8.40.1 (#2394)
  fix: cocoa sdk version updater (#2392)
  Send Less Client Reports When Rate Limited (#2380)
  build(deps): bump ruby/setup-ruby from 1.197.0 to 1.199.0 (#2386)
  chore(deps): update Native SDK to v0.7.12 (#2390)
  chore(deps): update Android SDK to v7.16.0 (#2373)
  fix build error for latest flutter beta (3.27.0) (#2385)
  Remove duplicate tests in sentry_client_test.dart (#2378)
  Handle backpressure earlier in pipeline (#2371)
  Add screenshot to `SentryFeedbackWidget` (#2369)

# Conflicts:
#	flutter/lib/src/event_processor/screenshot_event_processor.dart
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants